fix: send/receive asset selection should handle unsupported assets (#3311) - #12526
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds a shared wallet-aware asset filter. Popular and portfolio asset searches use it with asset predicates, wallet-chain restrictions, and updated memoization dependencies. Tests cover connected and disconnected wallets, unsupported-asset settings, and predicates. ChangesAsset search filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/TradeAssetSearch/TradeAssetSearch.tsx (2)
218-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for wallet-supported portfolio filtering.
Please add or verify tests for these cases:
- A connected wallet with
allowWalletUnsupportedAssets === falseexcludes unsupported chains.- A connected wallet with
allowWalletUnsupportedAssets === trueretains those assets.- No connected wallet does not filter by
walletConnectedChainIds.Use assets with both supported and unsupported
asset.chainIdvalues. This directly covers the back-navigation regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TradeAssetSearch/TradeAssetSearch.tsx` around lines 218 - 222, Add regression tests for the portfolio filtering logic in TradeAssetSearch, covering supported and unsupported asset.chainId values. Verify that a connected wallet excludes unsupported chains when allowWalletUnsupportedAssets is false, retains them when true, and that no connected wallet leaves all assets unfiltered by walletConnectedChainIds.
215-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit types to the new filter callbacks.
The callbacks at Line 215 and Line 220 rely on contextual inference. Add
(asset: Asset): booleanto both callbacks.As per coding guidelines, TypeScript function parameters and return values must be explicit.
Proposed change
- asset => assetFilterPredicate?.(asset.assetId) ?? true, + (asset: Asset): boolean => assetFilterPredicate?.(asset.assetId) ?? true, ... - asset => walletConnectedChainIds.includes(asset.chainId), + (asset: Asset): boolean => walletConnectedChainIds.includes(asset.chainId),Run
pnpm run lint --fixandpnpm run type-checkafter the change.Also applies to: 220-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TradeAssetSearch/TradeAssetSearch.tsx` at line 215, Update both filter callbacks in the TradeAssetSearch filtering logic (the callbacks at the referenced lines) to explicitly declare the parameter as Asset and the return type as boolean, including the callback using assetFilterPredicate?. Preserve their existing filtering behavior, then run pnpm run lint --fix and pnpm run type-check.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/TradeAssetSearch/TradeAssetSearch.tsx`:
- Around line 218-222: Add regression tests for the portfolio filtering logic in
TradeAssetSearch, covering supported and unsupported asset.chainId values.
Verify that a connected wallet excludes unsupported chains when
allowWalletUnsupportedAssets is false, retains them when true, and that no
connected wallet leaves all assets unfiltered by walletConnectedChainIds.
- Line 215: Update both filter callbacks in the TradeAssetSearch filtering logic
(the callbacks at the referenced lines) to explicitly declare the parameter as
Asset and the return type as boolean, including the callback using
assetFilterPredicate?. Preserve their existing filtering behavior, then run pnpm
run lint --fix and pnpm run type-check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecde0be5-9c79-4ece-88e6-919cd8a87601
📒 Files selected for processing (1)
src/components/TradeAssetSearch/TradeAssetSearch.tsx
Same predicate-then-chain-filter semantics, now shared with the portfolio list via the helper introduced in this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The helper applies the caller predicate plus wallet-support policy, not generic chain filtering, and the sibling hooks/ dir uses flat files. Also drop redundant parameter/return annotations per local idiom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.test.ts (4)
9-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename immutable test fixtures to UPPER_SNAKE_CASE.
Rename
supportedAsset,unsupportedAsset, andassetsto descriptive constant names such asSUPPORTED_ASSET,UNSUPPORTED_ASSET, andTEST_ASSETS.As per coding guidelines, "Use UPPER_SNAKE_CASE for constants and configuration values with descriptive names."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.test.ts` around lines 9 - 19, Rename the immutable test fixtures supportedAsset, unsupportedAsset, and assets to descriptive UPPER_SNAKE_CASE constants such as SUPPORTED_ASSET, UNSUPPORTED_ASSET, and TEST_ASSETS, and update all references in the test accordingly.Source: Coding guidelines
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unchecked
Assetassertions.Lines 12 and 17 claim that incomplete objects are
Assetvalues. This hides missing required fields from type checking.Make
filterAssetsByChaingeneric overPick<Asset, 'assetId' | 'chainId'>, preserve the generic type in its return value, and declare these fixtures with that explicit type. Do not cast them toAsset.After the change, run
pnpm run lint --fixandpnpm run type-check.As per coding guidelines, "NEVER use type assertions without proper validation in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.test.ts` around lines 9 - 17, Update filterAssetsByChain to be generic over Pick<Asset, 'assetId' | 'chainId'> and preserve that generic type in its return value. In the test fixtures, explicitly type supportedAsset and unsupportedAsset as the required Pick shape and remove the unchecked Asset assertions; then run pnpm run lint --fix and pnpm run type-check.Source: Coding guidelines
54-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the predicate parameter and return types.
Add an explicit
AssetIdtype forassetIdand an explicitbooleanreturn type for the callback.- assetFilterPredicate: assetId => assetId === ethAssetId, + assetFilterPredicate: (assetId: AssetId): boolean => assetId === ethAssetId,As per coding guidelines, "ALWAYS use explicit types for function parameters and return values in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.test.ts` around lines 54 - 61, Update the assetFilterPredicate callback in the test to explicitly type its assetId parameter as AssetId and its return value as boolean, reusing the existing AssetId type import or symbol.Source: Coding guidelines
21-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the omitted optional flag.
allowWalletUnsupportedAssetsis optional. When it is omitted,!allowWalletUnsupportedAssetsenables wallet-chain filtering. Add a case that omits this property and expects onlySUPPORTED_ASSET.Proposed test
+ it('excludes unsupported chains when allowWalletUnsupportedAssets is omitted', () => { + const result = filterAssetsByChain({ + assets: TEST_ASSETS, + hasWallet: true, + walletConnectedChainIds: [KnownChainIds.EthereumMainnet], + }) + + expect(result).toEqual([SUPPORTED_ASSET]) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.test.ts` around lines 21 - 30, The existing test only covers allowWalletUnsupportedAssets set to false; add a separate case for filterAssetsByChain that omits this optional property while keeping the wallet connected and walletConnectedChainIds restricted to EthereumMainnet, and assert the result contains only supportedAsset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.test.ts`:
- Around line 9-19: Rename the immutable test fixtures supportedAsset,
unsupportedAsset, and assets to descriptive UPPER_SNAKE_CASE constants such as
SUPPORTED_ASSET, UNSUPPORTED_ASSET, and TEST_ASSETS, and update all references
in the test accordingly.
- Around line 9-17: Update filterAssetsByChain to be generic over Pick<Asset,
'assetId' | 'chainId'> and preserve that generic type in its return value. In
the test fixtures, explicitly type supportedAsset and unsupportedAsset as the
required Pick shape and remove the unchecked Asset assertions; then run pnpm run
lint --fix and pnpm run type-check.
- Around line 54-61: Update the assetFilterPredicate callback in the test to
explicitly type its assetId parameter as AssetId and its return value as
boolean, reusing the existing AssetId type import or symbol.
- Around line 21-30: The existing test only covers allowWalletUnsupportedAssets
set to false; add a separate case for filterAssetsByChain that omits this
optional property while keeping the wallet connected and walletConnectedChainIds
restricted to EthereumMainnet, and assert the result contains only
supportedAsset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a26d71ba-6707-437c-b4c5-dbf5aafc9f31
📒 Files selected for processing (3)
src/components/TradeAssetSearch/TradeAssetSearch.tsxsrc/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.test.tssrc/components/TradeAssetSearch/helpers/filterAssetsByChain/filterAssetsByChain.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/TradeAssetSearch/TradeAssetSearch.tsx
|
Thanks for this fix — good catch, and the regression tests are appreciated. Verified the root cause: with a zero-balance (or freshly connected) wallet, the portfolio list falls back to the full asset universe ( Heads-up that we pushed a few small commits onto your branch while validating, so pull before making further changes:
Nothing behavioral changed in your fix itself. Should be good to merge once CI runs. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kaladinlight
changed the title
2 participants
Fixes #3311
When navigating within an asset view, opening the send/receive modal, and clicking back, the full list of all supported assets is displayed—including assets unsupported by the user's connected wallet.
This PR fixes the issue by ensuring that the fallback \portfolioAssetsSortedByBalance\ is properly filtered against \walletConnectedChainIds\ when \�llowWalletUnsupportedAssets\ is falsy (which is the case for Send/Receive modals).
Summary by CodeRabbit
Bug Fixes
Tests